-
Notifications
You must be signed in to change notification settings - Fork 170
perf: Add __slots__ to all DTypes
#3194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
I'm thinking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dangotbanned - I am quite a big fan of slots so I am definitely onboarded π
I left a comment to (hopefully) lower some of the maintenance effort
Noticed how slow it was during debugging (too much repeating)
Bug discovered by @FBruzzesi #3194 (comment) Now I need to fix 200/222 failures π
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dangotbanned - I left a non-bloking nitpick comment for the error message but feel free to ignore it if you don't like it much better.
Co-authored-by: Francesco Bruzzesi <[email protected]>
|
Sorry for asking here (I'm also not a maintainer of this library, but just sumbled over this issue): Instead of manually having to explicitly set Sorry again, just a curious question from an outsider! |
Hey @jonded94 - no need to apologize, actually thank you for questioning these kind of choices. We should definitely not dive into a solution without thinking about pros and cons of different approaches.
I feel you, I tried to propose a metaclass that can lower such burden :) and #3201 is using something towards such direction, so let's see if it makes it in the main branch soon as well.
We probably give it for granted in many places, but the main idea is that we want to have an API which is very close to the polars one. That's not always possible, but I would rather not have a discrepancy such as: from dataclasses import is_dataclass
from polars import DataType
from narwhals.dtypes import DType
is_dataclass(DataType)
# False
is_dataclass(DType)
# Trueor similarly: from collections import namedtuple
import polars as pl
Int64 = namedtuple('Int64', field_names=())
isinstance(pl.Int64(), tuple), isinstance(Int64(), tuple)
# False, TrueI hope it makes sense π |

Closes #3185
What type of PR is this? (check all applicable)
If you have comments or can explain your changes, please do so below
nw.dtypesCheck ifstable.*needs empty__slots__(It did)